Fixup funder fee buffer #1364
Fixup funder fee buffer #1364
Conversation
| val funderFeeBuffer = commitTxFeeMsat(commitments1.remoteParams.dustLimit, reduced.copy(feeratePerKw = 2 * reduced.feeratePerKw)) + htlcOutputFee(2 * reduced.feeratePerKw) | ||
| // NB: increasing the feerate can actually remove htlcs from the commit tx (if they fall below the trim threshold) | ||
| // which may result in a lower commit tx fee; this is why we take the max of the two. | ||
| val missingForSender = reduced.toRemote - commitments1.remoteParams.channelReserve - (if (commitments1.localParams.isFunder) fees.max(funderFeeBuffer.truncateToSatoshi) else 0.sat) |
There was a problem hiding this comment.
note that this is not what c-lightning has implemented: https://github.com/ElementsProject/lightning/blob/af7e87930878117a0633bb7a8cd4488b8e9c5b6d/channeld/full_channel.c#L430-L440
they trim htlcs at the current feerate, and then double the feerate and not trim again
There was a problem hiding this comment.
And what do you think of it? I think their choice is a bit weird and the additional reserve it ends up adding may be too big, but that's just my opinion.
There was a problem hiding this comment.
If we were to plot the commitment tx fees as a function of feerate, because of trimming, I expect it would be a spiky curve. This PR atm (sort of) takes the max of two samplings, while c-lightning takes a conservative upper bound.
As feerates change, the max of two samplings might be lower than the actual commitment tx fees, and potentially the issue the additional reserve is trying to fix could resurface (i.e. the fee spike buffer that is supposed to handle fee spikes up to 2x of current feerate might not be sufficient to handle all feerates in between). Though admittedly I did not look at specific numbers so perhaps this is highly unlikely in practice.
My personal opinion is that it might make more sense to take an upper bound of the curve, as c-lightning does, as then that should cover all possible feerates up to the chosen parameter (of "2x").
There was a problem hiding this comment.
If we were to plot the commitment tx fees as a function of feerate, because of trimming, I expect it would be a spiky curve.
Yes exactly, and that can be surprising at first!
the issue the additional reserve is trying to fix could resurface (i.e. the fee spike buffer that is supposed to handle fee spikes up to 2x of current feerate might not be sufficient to handle all feerates in between).
Yes but only temporarily, because the HTLCs you have in the commit tx will eventually settle.
Once they settle that gives you money back and unblocks the channel. And I think that's an important insight.
Taking the conservative upper bound also works, but it means there's more of your channel balance that you just can't use, which I want to avoid unless absolutely necessary. It's already hard to tell users "you put X mBTC in this channel but in fact what you'll be able to use is less than that, but trust us, this is necessary", so let's not add more to that if we can avoid it. Does that make sense?
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
Outdated
Show resolved
Hide resolved
t-bast
force-pushed
the
stuck-channels-fixup
branch
from
f981f63 to
fbe4529
Compare
|
What should we do about this PR @pm47? We haven't noticed the stuck channel issue at all for months, which means the previous fix is enough to mitigate it. |
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
Outdated
Show resolved
Hide resolved
We were previously only counting the additional HTLC at twice the current feerate. An HTLC in isolation doesn't make much sense: the feerate applies to the whole commit tx. Our fee buffer needs to account for a x2 feerate increase on the commit tx + an additional htlc. Note that this introduces another subtlety. The commit tx fee at twice the current feerate may actually be lower than the commit tx fee at the current feerate (if the commit tx contains many htlcs that are only slightly above the trim threshold).
t-bast
force-pushed
the
stuck-channels-fixup
branch
from
fbe4529 to
97c9b57
Compare
See ACINQ/eclair#1364 for details. We are now spec-compliant.
* Remove unused check in Commitments.sendAdd * Clean-up commitments * Update funder fee buffer (ACINQ/eclair#1364)
| // - commit tx fee at twice the feerate = 724 * 2000 = 1448 sat (all HTLCs have been trimmed) | ||
| // - cost of an additional untrimmed HTLC = 172 * 2000 = 344 sat | ||
| // - funder fee buffer = 1448 + 344 = 1792 sat | ||
| // In that case the current commit tx fee is higher than the funder fee buffer and will dominate the balance restrictions. |
There was a problem hiding this comment.
I think a fee-buffer can act in two different directions, meaning that in case of a down-spike former dust htlcs can use this buffer as well. So I would argue to not really dynamically simulate a 2x increase in the feerate and based of this calculate the feebuffer but rather just take the worst case scenario and account for the double fee at the current feerate (thats basically a 2x simulation without taking any trimmed htlcs into account) ? Why not consider tripped htlcs? Because trimmed htlcs are accounted for fees anyways. So in your above example when all 10 htlcs are trimmed, we have a final fee of
0.724 * 2000 + 10 * 1250 = 13950 sasts, so we could just keep the double fee as a buffer because this would have the advantage, if there are already trimmed away htlcs on the commitment, the feerate could also be decreased in an event of a fee-dump ? wdyt ?
We were previously only counting the additional HTLC at twice
the current feerate.
An HTLC in isolation doesn't make much sense: the feerate
applies to the whole commit tx. Our fee buffer needs to account
for a x2 feerate increase on the commit tx + an additional htlc.
Note that this introduces another subtlety. The commit tx fee at twice the
current feerate may actually be lower than the commit tx fee at the current
feerate (if the commit tx contains many htlcs that are only slightly above
the trim threshold).
See lightning/bolts#740 (comment) for context.